-
Notifications
You must be signed in to change notification settings - Fork 92
fix: fail build/deploy when using not yet unsupported Node.js Midleware #3016
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📊 Package size report 0.03%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
139d813
to
ff5ee51
Compare
// The canary versions are out of band, as there stable/latest patch versions higher than base of canary versions | ||
// and change was not backported to stable versions | ||
return nextVersionSatisfies('>=15.4.2-canary.33') && isNextCanary() | ||
// The canary versions are out of band, as there stable/latest patch versions higher than base of canary versions without | ||
// the change included | ||
return nextVersionSatisfies(isNextCanary() ? '>=15.4.2-canary.33' : '>=15.5.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to Node.js middleware, but rather new stable release - just adding it here to avoid separate PR that would slow down merging things in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// https://github.com/vercel/next.js/blob/8367faedd61501025299e92d43a28393c7bb50e2/packages/next/src/build/index.ts#L2465 | ||
// Node.js Middleware has hardcoded /_middleware path | ||
if (manifest.functions['/_middleware']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oof, there's no more explicit way to check this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:S It's literally what Next.js uses internally as well https://github.com/search?q=repo%3Avercel%2Fnext.js%20%22%2F_middleware%22&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of any better way, so I just followed what Next.js does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, now that I think of it - possibly "nicer" way would be to actually use adapters api to get list of "outputs" where middleware should exist and we could find middleware entry and we could check, but adapters api is definitely not stable, so I wouldn't want to actually use it in production yet, but that's the only idea that comes to mind that doesn't involve checks like above
Description
We currently don't support Node.js middleware properly. It is being bundled and executed as part of serverless handler. It actually works fine for non-cacheable routes, but contract is broken for cacheable pages where middleware would be executed once before route is cached, but would not execute for future requests against cached route and middleware won't get executed for static assets requests.
So to avoid confusion this add specific failure to prevent broken deploys using node middleware until there is support for it with following error:
There is work happening on actual support in #3018 which is the reason we know already about limitations. It's not fully finished yet tho, so until that happens we should prevent broken deploys from people that might be experimenting with it as it seems to be working in some cases today and it might be not apparent that it doesn't work fully.
Documentation
Tests
Added test asserting explicit error is thrown
Relevant links (GitHub issues, etc.) or a picture of cute animal